feat(mobile): use efficient sync #8842
Conversation
Deploying immich with
|
| Latest commit: |
db6a66b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6dd0596e.immich.pages.dev |
| Branch Preview URL: | https://feat-app-efficient-sync.immich.pages.dev |
| for (int i = 0;; i += chunkSize) { | ||
| DateTime? lastCreationDate; | ||
| String? lastId; | ||
| for (;;) { |
There was a problem hiding this comment.
It's identical to while(true). I can change it but it does not really matter I'd say :)
There was a problem hiding this comment.
I see this a lot in embedded system code, not used to seeing it in higher level language 😝
There was a problem hiding this comment.
I personally use this for loops when I break out using other means.
While(true).. I use for endless tasks. Like a server listening indefinitely until it's killed etc.
There was a problem hiding this comment.
Can this be rewritten to a do while loop on the condition assets.length < chunkSize? I think that's the only break condition I see here. I concur that while (true) loops are odd and potentially dangerous. I also think for (;;) is less readable. I did not know what it meant until I read some of the other comments.
There was a problem hiding this comment.
sadly, I can't put the break condition in the loop condition check: it would not even run the first pass of the loop - i can only to while(true) instead of for(;;). The loop has two break conditions that cannot be handled in the loop header
|
I ran into this error on the server when running the PR
Here is the mobile error log |
I would guess this is caused by this: |
It seems some more server changes are required for stacked assets (my test instance probably had no stacked assets at the time...). I converted this PR back to a draft and will work on a new PR for the required server changes. |
fyfrey
force-pushed
the
feat/app-efficient-sync
branch
from
71fc321 to
be7865b
Compare
|
Hi @fyfrey I tested this again and found a problem with rendering memory lane. Here is the error message
From my debugging, the API returned the correct data. Still, the call to get the asset from the database when building the model probably happened before the synchronization process finished, so it could not parse all data correctly, leading to the error. Or, similar to previous symptom, not all assets are fetched
|
Thanks @alextran1502 ! I've pushed a new commit to fix the bug in the memory lane. We were just lucky it did not occur previously.. |
|
Hi @fyfrey, the change doesn't entirely fix the issue. I have multiple memories on a day but only the first memory gets rendered, so I checked the database. On |
Thank you for testing again @alextran1502 . Very interesting. I did not notice any sync discrepancy (asset count) with my initial server changes. In the newer fix PR (#9100) we changed it to use the common asset query builder. It turns out it is incompatible with the app with stacked assets as the app expects all assets individually while the server returns them in an array with of the parent asset. This is probably the same issue you encountered a while ago and reverted server refactoring changes in #7752. made another server PR to fix it: #9189 to test: run server with #9189 and mobile with this PR... or locally merge one into the other.. simply copy the few changes from #9189 into your local branch of this PR :) |
|
I tried again with #9189 on my prod (FOR SCIENCE!!) and I still see missing memories. Checking the database again, now I see 68000 assets are fetched on this PR while still 69000 assets on |
|
Something must still be different between the old and the new server query. The old method also fetched archived assets for partners, maybe this accounts for the difference. But memories should not show archived assets.... I'll take another look at the code, or better the generated SQL. Edit: |
follow-up for #8755
enable the use of the more efficient endpoints to perform sync operations.
only merge after #9189 has been in a release for 1-2 weeks.